Skip to content

PERF: Skip header check in parquet reader - #22679

Merged
rapids-bot[bot] merged 8 commits into
NVIDIA:mainfrom
TomAugspurger:tom/parquet-header-removal
May 29, 2026
Merged

PERF: Skip header check in parquet reader#22679
rapids-bot[bot] merged 8 commits into
NVIDIA:mainfrom
TomAugspurger:tom/parquet-header-removal

Conversation

@TomAugspurger

@TomAugspurger TomAugspurger commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

The parquet reader previously required three reads to read the parquet footer:

  1. A 4 byte read to check the header for the parquet magic bytes
  2. An 8 byte read to read the footer length and footer parquet magic bytes
  3. A varaible-length read for the footer metadata

We don't really care about ensuring that the header is valid. For high-latency storage, it's not worth the extra read.

Part of #22668, which also proposes to remove the second 8-byte read. But this is a smaller change that should be less controversial.

@TomAugspurger
PERF: Skip header check in parquet reader
0c744ac 
The parquet reader previously required 3 reads to read the parquet footer:

1. A 4 byte read to check the header for the parquet magic bytes
2. An 8 byte read to read the footer length and footer parquet magic bytes
3. A varaible-length read for the footer metadata

We don't really care about ensuring that the header is valid. For high-latency
storage, it's not worth the extra read.
@copy-pr-bot

copy-pr-bot Bot commented May 27, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label May 27, 2026
@TomAugspurger TomAugspurger added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 27, 2026
@TomAugspurger

TomAugspurger commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 0c744ac

@TomAugspurger
TomAugspurger requested a review from vuule May 28, 2026 12:15
@TomAugspurger
TomAugspurger marked this pull request as ready for review May 28, 2026 12:15
@TomAugspurger
TomAugspurger requested a review from a team as a code owner May 28, 2026 12:15
@TomAugspurger
TomAugspurger requested a review from bdice May 28, 2026 12:15
@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown

Actionable comments posted: 0

@coderabbitai

coderabbitai Bot commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 64e37fda-6acc-438d-ba5d-040dca1760a0

📥 Commits

Reviewing files that changed from the base of the PR and between 48bffa0 and 1b7f6a0.

📒 Files selected for processing (1)
  • cpp/tests/io/parquet_reader_test.cpp
📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation of Parquet file integrity by adjusting footer verification logic.

  • Tests

    • Added test case to verify proper error handling for corrupted Parquet files with invalid footer signatures.

Walkthrough

This PR refactors Parquet footer validation by removing an upfront header magic check in fetch_footer_to_host and adds a test that verifies corrupted footer magic is detected. Footer integrity is validated only via tail ender checks after reading.

Changes

Parquet Footer Validation Refactoring

Layer / File(s) Summary
Footer validation logic simplification
cpp/src/io/parquet/io_utils/parquet_io_utils.cpp		 Removes initial header read and header magic check from fetch_footer_to_host; validates datasource length then reads footer from datasource tail and validates ender->magic and ender->footer_len.
Footer magic validation test
cpp/tests/io/parquet_reader_test.cpp		 Adds ParquetReaderTest.InvalidFooterMagic which creates a Parquet payload in-memory, corrupts footer magic bytes with BAD!, and verifies cudf::logic_error is thrown.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

cuIO

Suggested reviewers

  • wence-
  • bdice
  • ttnghia
  • mroeschke
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: skipping the header magic check in the parquet reader for performance reasons, which aligns with the code modifications removing header validation logic.
Description check ✅ Passed The description is directly related to the changeset, explaining the performance rationale for removing the header check and providing context about the three-read process and high-latency storage concerns.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@TomAugspurger
TomAugspurger requested a review from a team as a code owner May 28, 2026 16:45
@TomAugspurger
TomAugspurger requested review from rjzamora and wence- May 28, 2026 16:45
@github-actions github-actions Bot added Python Affects Python cuDF API. pylibcudf Issues specific to the pylibcudf package labels May 28, 2026
@GPUtester GPUtester moved this to In Progress in cuDF Python May 28, 2026
@coderabbitai

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown

Actionable comments posted: 0

vuule
vuule approved these changes May 28, 2026
View reviewed changes
Comment thread cpp/tests/io/parquet_reader_test.cpp Outdated
@vyasr
vyasr removed the request for review from a team May 29, 2026 01:26
@vyasr
vyasr removed request for rjzamora and wence- May 29, 2026 01:26
@TomAugspurger

TomAugspurger commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit 797d730 into NVIDIA:main May 29, 2026
117 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in cuDF Python May 29, 2026
@coderabbitai coderabbitai Bot mentioned this pull request May 29, 2026
Merged
3 tasks
@TomAugspurger TomAugspurger mentioned this pull request Jun 5, 2026
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bdice bdice bdice approved these changes
@vuule vuule vuule approved these changes

Assignees

@TomAugspurger TomAugspurger

Labels

improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@TomAugspurger @bdice @vuule @GPUtester